[TrimmableTypeMap] Remove ForceUnconditionalEntries workaround#11345
Open
simonrozsival wants to merge 20 commits into
Open
[TrimmableTypeMap] Remove ForceUnconditionalEntries workaround#11345simonrozsival wants to merge 20 commits into
simonrozsival wants to merge 20 commits into
Conversation
Restore conditional TypeMap entries for non-essential MCW bindings now that the runtime trimmer issue has been fixed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the ForceUnconditionalEntries workaround from trimmable typemap model generation, restoring conditional 3-arg TypeMapAttribute emission for non-essential MCW bindings while keeping essential runtime/user ACW entries unconditional.
Changes:
- Removes the global force-unconditional switch from
ModelBuilder. - Restores conditional target references for non-essential MCW entries.
- Updates model and PE blob tests to expect restored 3-arg behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ModelBuilder.cs |
Removes workaround logic and reverts unconditional decisions to IsUnconditionalEntry() plus alias-specific rules. |
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapModelBuilderTests.cs |
Updates assertions and test names/comments for restored conditional MCW typemap entries. |
…e name collision - Replace [DynamicDependency] attributes on FindX509TrustManager with a HackToPreserveInvokers method using [MethodImpl(NoInlining)] to prevent the linker from trimming IX509TrustManagerInvoker and X509ExtendedTrustManagerInvoker. - Remove obsolete API 21-23 Conscrypt TrustManagerImpl workaround. - Rename generated enum from AppFunctionState to AppFunctionEnabledState to avoid name collision with the new Android API 37 android.app.appfunctions.AppFunctionState class. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The isinst instruction properly preserves conditional TypeMap entries for interfaces through the MarkType -> MarkRequirementsForInstantiatedTypes -> ProcessType chain in ILLink. The typeof() and [DynamicDependency] additions were based on an incorrect analysis that ProcessType was skipped for interfaces - while true in the isinst handler's inner switch, MarkType itself calls MarkRequirementsForInstantiatedTypes for interfaces which calls ProcessType without any interface skip. Verified with both ILLink and NativeAOT using a multi-assembly repro that mirrors the Android typemap assembly layout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When the getSuperclass() class hierarchy walk fails to find a targetType-compatible TypeMap proxy (e.g., because an intermediate class entry like X509ExtendedTrustManager was trimmed), fall back to walking Java interfaces via getInterfaces() at each level of the class hierarchy. This allows the runtime to find TypeMap entries for Java interfaces (e.g., javax/net/ssl/X509TrustManager) that are preserved by the trimmer but unreachable via getSuperclass() alone. The interface walk only runs when targetType is an interface, keeping the common path (class-based resolution) unchanged. Also adds a device test TrustManagerFactory_GetTrustManagers_ReturnsIX509TrustManager that verifies TrustManagerFactory.GetTrustManagers() returns elements that can be cast to IX509TrustManager. This test runs on both llvm-ir and trimmable typemap paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…es method ID - Check interfaces inline during the getSuperclass() walk instead of a separate second pass, avoiding redundant GetObjectClass/getSuperclass calls - Cache the JNI method ID for getInterfaces() in a static field - Rename TryGetProxyFromInterfacesOfClass to TryMatchInterfaces Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…aces scope Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ound The [DynamicDependency] attributes on FindX509TrustManager are needed by both llvm-ir and trimmable typemap paths to preserve invoker types. Without them, the llvm-ir linker also trims X509ExtendedTrustManager entries, causing the same IX509TrustManager resolution failure. Removed the API 21-23 Conscrypt TrustManagerImpl workaround since minimum supported API level is now higher. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address review feedback: add coverage that all-MCW alias groups emit a conditional (3-arg) base alias-holder entry, mixed ACW/MCW alias groups stay unconditional (2-arg), and essential runtime types remain unconditional regardless of peer types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace raw JNIEnv.GetMethodID/CallObjectMethod/GetArrayLength/ GetObjectArrayElement/DeleteLocalRef with their JniEnvironment equivalents (JniMethodInfo, JniObjectReference, JniEnvironment.Arrays, JniEnvironment.InstanceMethods). This matches the style of the surrounding hierarchy walk code and uses proper ref disposal via JniObjectReference.Dispose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Wrap the JNI class handle in Java.Lang.Class with DoNotTransfer ownership and call the existing GetInterfaces() binding. This reuses the JniPeerMembers method ID caching, thread-safe lookup, and JNI remapping that the generated binding already handles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… test - Use DoNotTransfer | DoNotRegister when wrapping JNI class handles in Java.Lang.Class to avoid peer table registration overhead - Remove extra blank line before GetProxyForJavaObject - Remove verbose diagnostic logging from device test, keep only the assertion with descriptive failure message - Remove unused Log import from test - Keep hierarchy walk using JniObjectReference (not Java.Lang.Class) to avoid potential recursion into TypeMap during class resolution Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Java.Lang.Class.GetInterfaces() returns Java.Lang.Class[] where each element holds a global ref. Dispose them in a finally block to avoid leaking grefs until GC collection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid wrapping Java class handles in temporary managed peers while matching Java interfaces in the trimmable typemap path. This prevents disposing shared class peers and adds regression coverage for the interface proxy lookup path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use JniEnvironment APIs consistently while walking Java interfaces and cache interface lookup results by Java class and target interface. This avoids repeated JNI reference churn in hot interface marshalling paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Include the trust-manager marshalling test in Mono.Android.NET-Tests and add an API-independent Java fixture that exercises base-interface return values whose concrete implementation advertises a derived interface. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add the missing Microsoft.Android.Runtime import so RuntimeFeature resolves in TrustManagerMarshallingTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remove the
ForceUnconditionalEntriesworkaround from the trimmable typemap generator and fix the runtime type-resolution gap it was masking.This restores conditional
TypeMapAttributeentries for non-essential bindings while keeping interface-based Java objects marshalable to the most specific managed invoker type that survived trimming.Background
ForceUnconditionalEntrieswas a temporary workaround for dotnet/runtime#127004. It forced all typemap entries to be emitted unconditionally, which prevented the trimmer from removing unused framework bindings.When conditional entries were restored,
TrustManagerFactory.GetTrustManagers()could return anITrustManagerInvokerinstead of anIX509TrustManagerInvokerin the trimmable typemap path. The preserved managed entry forjavax/net/ssl/X509TrustManagerwas not reachable because the runtime only walked the Java class hierarchy; Java interfaces were not considered.Fix
ForceUnconditionalEntriesso non-essential MCW entries are emitted conditionally again.TrimmableTypeMapwhen the requested target type is an interface.Class.getInterfaces().(className, targetType), including misses.JniEnvironment/JniObjectReferenceAPIs with explicit disposal for JNI references.TargetTypeMatches()based on Java assignability checks so the linker does not need to preserve all managed interface metadata.Other changes
[DynamicDependency]attributes onFindX509TrustManager; both typemap paths need the invoker types kept alive.TrustManagerImplworkaround.AppFunctionStatetoAppFunctionEnabledStateto avoid the Android API 37android.app.appfunctions.AppFunctionStatename collision.TrustManagerFactory_GetTrustManagers_ReturnsIX509TrustManagercoverage.ExtendedValueProviderandInterfaceMarshalling). Their C# APIs are generated from@(AndroidJavaSource)bindings and are used byJavaInterfaceLookup_BaseInterfaceReturnType_UsesDerivedInterfaceProxy.InstallAndroidDependenciesTestnow that the Xamarin manifest contains the requested component.InterfaceProxyLookup_DoesNotLeakGlobalRefsfor now because unrelated global-reference growth can poison the result.Validation
Covered by the typemap generator tests, typemap runtime tests, and Mono.Android.NET device tests in CI. The direct gref-counting regression test was intentionally removed until the unrelated global-reference growth can be isolated.